Fix inconsistent reg struct layout for 64bit tracer and 32bit tracee#2448
Fix inconsistent reg struct layout for 64bit tracer and 32bit tracee#2448Evian-Zhang wants to merge 6 commits intonix-rust:masterfrom
Conversation
| /// on aarch64 and riscv64. | ||
| /// | ||
| /// Currently, in x86_64 platform, if the tracer is 64bit and tracee is 32bit, this function | ||
| /// will return an error. |
There was a problem hiding this comment.
Does it? Did you forget to add a size check below?
There was a problem hiding this comment.
Oh yes, I forgot this check. But as stated in the man page, PTRACE_GETREGS does not return the size of bytes written, so it seems that there is no easy way to check this?
There was a problem hiding this comment.
Do you know how C programs use PTRACE_GETREGS for 32-bit targets, then?
There was a problem hiding this comment.
As far as I know, we should check the target tracee bitness in advance, instead of checking after the syscall returns. Is it better to mark this method unsafe and encourage users to use getregset for safe tracee register retrieval?
There was a problem hiding this comment.
If it cannot guarantee that the return value will be correctly constructed, then yes it must be unsafe. Either that, or it could return a union (which is unsafe to read from).
There was a problem hiding this comment.
Updated. I prefer to mark it as unsafe, since it is hard to import the 32bit user_regs_struct in libc crate.
There was a problem hiding this comment.
As far as I know, we should check the target tracee bitness in advance, instead of checking after the syscall returns.
From this post, it is not trivial to check the bitness, so I would also prefer to mark this function unsafe as well if we don't have a better option.
since it is hard to import the 32bit user_regs_struct in libc crate
Would you like to elaborate on this a bit?
There was a problem hiding this comment.
If we return a union, then this union should involve both 64bit and 32bit version of user_regs_struct, which is provided by libc crate instead of the nix itself. However, the struct definition is bound to the compile target of libc crate, we cannot import a 32bit user_regs_struct in libc if we compiles to x86_64
There was a problem hiding this comment.
However, the struct definition is bound to the compile target of libc crate, we cannot import a 32bit user_regs_struct in libc if we compiles to x86_64
Right, the actual type is decided at compile time, so we cannot use the 32-bit version unless we define it in Nix, which is not something Nix should do.
$ pwd
libc/src/unix/linux_like/linux/gnu
$ rg "struct user_regs_struct"
b64/x86_64/mod.rs
178: pub struct user_regs_struct {
b32/x86/mod.rs
74: pub struct user_regs_struct {
b64/loongarch64/mod.rs
193: pub struct user_regs_struct {
b64/riscv64/mod.rs
196: pub struct user_regs_struct {
b32/riscv32/mod.rs
200: pub struct user_regs_struct {
b64/aarch64/mod.rs
146: pub struct user_regs_struct {As far as I know, we should check the target tracee bitness in advance, instead of checking after the syscall returns.
After figuring out the bitness, how do you use a 32-bit version structure in C if you know the tracee is a 32-bit process? Since in C, these types are also chosen at compile time with #ifdef macros.
There was a problem hiding this comment.
asomers
left a comment
There was a problem hiding this comment.
Could you please add a changelog entry? Also, have you audited the other ptrace functions for similar behavior?
I'll add one. For other ptrace functions, it is the |
|
What about the ptrace functions in the BSD module? Did you look at them? |
I do not have a BSD machine, and cannot check whether it has the same behavior with Linux |
…e and tracer; Add changelog since ptrace::getregs is now unsafe
What does this PR do
Fix #2447
Checklist:
CONTRIBUTING.md